Unify interpreter and witness generation logic. #56
Conversation
| let get_vals = |opt_vals: &[Option<U256>]| { | ||
| opt_vals | ||
| .iter() | ||
| .map(|&elt| match elt { | ||
| Some(val) => val, | ||
| None => U256::zero(), | ||
| }) | ||
| .collect::<Vec<U256>>() |
There was a problem hiding this comment.
There's a lot of this stuff going on in the PR, a consequence of using Option<U256>. Are we sure we can't avoid wrapping values in Option? Can we just use 0 or U256::MAX or some other trickery?
There was a problem hiding this comment.
The problem is that we need to know whether a certain memory spot was set or not. But memory values can take any U256 value, so unfortunately, I think we do need the Option<U256>...
But ad-hoc code might be useful for testing macross or any code not in the kernel. |
Maybe we can revisit this when we have a concrete need for it? Or is this a blocker in your opinion? |
I'm not strong about this. The only problem could be that when we have this test (I have something like that on another branch) we would need to come back to the previous more general version of the interpreter where the code can be anything. But maybe there's an easy trick that I can't see now (here's the test) |
| } | ||
| } | ||
|
|
||
| /// Returns the value of a `State`'s clock. |
There was a problem hiding this comment.
Maybe not in this PR, but we should unify the clock someday. I think we should add a clock register for the prover too, but it's not a strong opinion either.
* Add traits State and Transition * Move perform_state_op to Transition * Fix bug * Get rid of is_generation_state + add reviews * Remove TDO * Fix handle_error * Move preinitialized_segments from the interpreter to MemoryState * Clippy * Apply get_halt_context comment --------- Co-authored-by: Linda Guiga <lindaguiga3@gmail.com>
| @@ -87,7 +87,7 @@ fn test_jumpdest_analysis() -> Result<()> { | |||
| .get_mut(&CONTEXT) | |||
| .unwrap() | |||
| .pop(); | |||
| interpreter.push(U256::one()); | |||
| interpreter.push(41.into()); | |||
There was a problem hiding this comment.
With the unification, the proofs are checked in the reverse order. I'm still not sure why, but it must come from a previous discrepancy between the interpreter and witness generation that we hadn't identified...
There was a problem hiding this comment.
cc @4l0n50 in case you have an explanation
There was a problem hiding this comment.
Hmmm. Maybe is because the jumpdests where sorted at some point but is not done anymore? (as it was no necessary)
| @@ -432,7 +483,7 @@ impl<F: Field> State<F> for GenerationState<F> { | |||
| fn incr_interpreter_clock(&mut self) {} | |||
There was a problem hiding this comment.
I think I'd rather have the trait define the blanket no-op impl, so that we don't have to keep a reference to something that's interpreter specific here.
There was a problem hiding this comment.
Actually, in the end, I think it's better to increment the interpreter clock in push_cpu, since this is when the generation "clock" is increased. This would get rid of that method altogether, and would insure a better unification (we noticed that the clock of the interpreter was not increased when calling generate_exception)
There was a problem hiding this comment.
Ah yeah it's even better
This PR aims at unifying the logic of the interpreter and that of the CPU, for two main reasons:
jumpdest_analysisduring generation,The discrepancy between the interpreter and CPU logic has led to some bugs in those two use cases, and the unification would make the debugging and usage much easier.
Another change in the
Interpreteris the removal of the lifetime:generation_state, so we use these instead of theInterpreter's specific fieldInterpreter's code is the kernel code, which means we can also remove thecodefield of theInterpreter(which also required a lifetime).Since one of the motivations for this PR is to prepare for continuation, it also includes the following changes, which make segment generation much easier:
MemoryStateare stored asOption<U256>,TrieDatasegment if we are in the interpreter, but instead stored in the new interpreter fieldpreinitialized_segments.Those two changes make it easier to determine accurately which parts of a memory have been accessed during one segment execution.